-
Notifications
You must be signed in to change notification settings - Fork 266
PHPLIB-1702: Always consult server encryptedFieldsMap when dropping collections with autoEncryption
enabled
#1745
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
55ec999
to
97284ec
Compare
Necessary to fix PyMongo compat with MongoDB 4.0
); | ||
|
||
// createEncryptedCollection should create three collections | ||
$this->assertCount($originalNumCollections + 3, $this->database->listCollectionNames()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the collection already exists before running the test, this assertion fails.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this only a concern when aborting the test locally (or retrying after a failure)?
No objections to dropping it before this first assertion.
@@ -83,6 +83,8 @@ class Client | |||
|
|||
private WriteConcern $writeConcern; | |||
|
|||
private bool $autoEncryptionEnabled; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created PHPC-2615 to allow us to fetch this from the Manager directly.
@@ -134,6 +136,7 @@ public function __construct(?string $uri = null, array $uriOptions = [], array $ | |||
$this->uri = $uri ?? self::DEFAULT_URI; | |||
$this->builderEncoder = $driverOptions['builderEncoder'] ?? new BuilderEncoder(); | |||
$this->typeMap = $driverOptions['typeMap']; | |||
$this->autoEncryptionEnabled = isset($driverOptions['autoEncryption']['keyVaultNamespace']); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted that keyVaultNamespace
is a required autoEncryption
option.
phongo_manager_set_auto_encryption_opts
doesn't seem to check about keyVaultNamespace
being unset, so I assume we'd rely on libmongoc(rypt) raising an error since it's documented as required. In any event, it's sensible to check for it directly instead of just seeing if autoEncryption
is set.
{ | ||
// No-op if the encryptedFieldsMap autoEncryption driver option was omitted | ||
if ($manager->getEncryptedFieldsMap() === null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, it seems like we'd want the Manager to be able to report whether autoEncryption is enabled.
autoEncryption
enabled
Necessary to fix change stream tests for server 8.2+
https://jira.mongodb.org/browse/PHPLIB-1702
This is a rebase of #1742 on v1.21 with additional modifications from @GromNaN's code review.